Deprecate make_scalar_function #8878

viirya · 2024-01-15T20:59:38Z

Which issue does this PR close?

Closes #8866.

Rationale for this change

make_scalar_function is easily to be misused and causing unexpected behavior (e.g., #8866) if users don't understand its underlying logic. Actually this function looks more like a crate private utility function we should not expose.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

datafusion/physical-expr/src/functions.rs

viirya · 2024-01-15T23:41:01Z

datafusion/physical-expr/src/functions.rs

-pub fn make_scalar_function<F>(inner: F) -> ScalarFunctionImplementation
+/// Note that this function makes a scalar function with no arguments or all scalar inputs return a scalar.
+/// That's said its output will be same for all input rows in a batch.
+pub(crate) fn make_scalar_function<F>(inner: F) -> ScalarFunctionImplementation


Or we should deprecate it and ask user to use ScalarUDFImpl instead.

I think it's a behavior change, so I add a label for it

jackwener

Make sense to me, thanks @viirya

alamb

Thanks @viirya -- I now understand the issue in #8866 better.

I think make_scalar_function is quite a nice function that lowered the cognative barrier to making udfs and it would be great if we could keep it. I think @jorgecarleitao or @andygrove originally added it when we introduced ColumnarValue so that new users didn't have to understand ColumnarValue to begin working on it.

That being said, perhaps encouraging people to use the trait ScalarUDFImpl is ok,

datafusion/physical-expr/src/functions.rs

alamb · 2024-01-16T11:23:57Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

+        let ColumnarValue::Array(array) = &args[0] else {
+            panic!()
+        };
+        Ok(ColumnarValue::Array(Arc::new(array.clone()) as ArrayRef))


I wonder if we could make this code easier to work with using From impls, like

Suggested change

Ok(ColumnarValue::Array(Arc::new(array.clone()) as ArrayRef))

Ok(ColumnarValue::from(Arc::new(array.clone()) as ArrayRef))

Maybe it doesn't matter 🤔

Okay, I added two From impls for ColumnarValue.

alamb · 2024-01-16T11:26:49Z

datafusion-examples/examples/simple_udf.rs

        // in DataFusion, all `args` and output are dynamically-typed arrays, which means that we need to:
        // 1. cast the values to the type we want
        // 2. perform the computation for every element in the array (using a loop or SIMD) and construct the result

        // this is guaranteed by DataFusion based on the function's signature.
        assert_eq!(args.len(), 2);

+        let ColumnarValue::Array(arg0) = &args[0] else {


I think the more correct pattern is to create an array with into_array

Suggested change

let ColumnarValue::Array(arg0) = &args[0] else {

let arg0 = args[0].into_array(num_rows)

Though now I wrote that it is not super clear to me where the num_rows should come from 🤔

Also, maybe this is more of the fix for #8866 -- that for a volatile function, it shouldn't be passed a scalar but unstead the array should be expanded prior to invocation

In what situations would arg of pow be a Scalar? If we can provide an example to reproduce it, our case/test can cover more condition.

I think something like pow(1.0, 2.0) (pass in literal values)

Need more discuss

Co-authored-by: Andrew Lamb <[email protected]>

alamb

Thanks @viirya --- I think it would be nice to avoid some of the repetition of converting [ColumnarValue] into [ArrayRef] by encapsulating it into a function but I also think we can do so as a follow on PR.

I also think we might want to consieer wait for DataFusion 35.0.0 to be released #8863 before merging this PR

alamb · 2024-01-17T20:37:13Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

-            let arg0 = as_int32_array(&args[0])?;
-            let arg1 = as_int32_array(&args[1])?;
+        let fun = Arc::new(|args: &[ColumnarValue]| {
+            let len = args


Maybe as a follow on PR we can make a function that does this length inference and conversion to array (mostly so it can be documented) to make the ideas easier to find 🤔

Yea, have the same thought too. It will be still useful for ScalarUDFImpl as invoke takes args: &[ColumnarValue] and users might need length inference too.

viirya · 2024-01-19T06:33:02Z

I also think we might want to consieer wait for DataFusion 35.0.0 to be released #8863 before merging this PR

Thanks @alamb. This currently marks make_scalar_function as deprecated since 35.0.0 (#[deprecated(since = "35.0.0"...), if we want to merge this after DataFusion 35.0.0 releases, should we change the deprecation message to since = "36.0.0")?

alamb · 2024-01-22T19:42:18Z

I am going to merge this PR on the assumption that the 35.0.0 release is in the final stages of voting and we won't need a new RC.

Thanks again @viirya

…unction

alamb · 2024-01-22T19:43:14Z

I took the liberty of merging up from main just to ensure there are no logical conflicts and plan to merge this PR when the CI checks pass

viirya · 2024-01-22T20:40:44Z

Thank you @alamb @jackwener

dadepo · 2024-01-23T08:20:32Z

The documentation here https://arrow.apache.org/datafusion/library-user-guide/adding-udfs.html probably needs updating too, as it still suggest to use make_scalar_function

viirya · 2024-01-23T08:51:08Z

Thanks. I will update the document together in a follow up with other stuffs (e.g., #8878 (comment)).

`make_scalar_function` has been deprecated since v36 [0]. It is being removed from the public api in v43 [1]. [0]: apache/datafusion#8878 [1]: apache/datafusion#12505

Make make_scalar_function private

256fa22

github-actions bot added physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 15, 2024

viirya commented Jan 15, 2024

View reviewed changes

datafusion/physical-expr/src/functions.rs Outdated Show resolved Hide resolved

viirya added 4 commits January 15, 2024 13:51

More

7ebd759

More

88fdc2c

Fix

1336a36

More

5399f6c

viirya commented Jan 15, 2024

View reviewed changes

jackwener added the api change Changes the API exposed to users of the crate label Jan 16, 2024

jackwener previously approved these changes Jan 16, 2024

View reviewed changes

alamb reviewed Jan 16, 2024

View reviewed changes

alamb mentioned this pull request Jan 16, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 15, 2024 #8864

Closed

9 tasks

viirya mentioned this pull request Jan 16, 2024

UDFs marked as volatile do not appear to evaluate multiple times for each output row. #8866

Closed

viirya and others added 3 commits January 16, 2024 23:27

Update datafusion/physical-expr/src/functions.rs

25e89c2

Co-authored-by: Andrew Lamb <[email protected]>

For review

e06ec90

For review

13578b4

github-actions bot added the logical-expr Logical plan and expressions label Jan 17, 2024

viirya changed the title ~~Make make_scalar_function private~~ Deprecate make_scalar_function Jan 17, 2024

Fix

d7b117a

alamb approved these changes Jan 17, 2024

View reviewed changes

Update deprecated since tag

64a7642

Merge remote-tracking branch 'apache/main' into private_make_scalar_f…

8e559a8

…unction

alamb merged commit 903ef94 into apache:main Jan 22, 2024
22 checks passed

This was referenced Jan 23, 2024

Fix clippy failures: error: use of deprecated function `functions::make_scalar_function #8972

Merged

Clippy failures on main: error: use of deprecated function functions::make_scalar_function` #8973

Closed

This was referenced Oct 11, 2024

fix: remove use of deprecated make_scalar_function apache/datafusion-python#906

Open

Add PyarrowScalarUDF and convert PyScalarUDF to API recommended upstream apache/datafusion-python#907

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate make_scalar_function #8878

Deprecate make_scalar_function #8878

viirya commented Jan 15, 2024

viirya Jan 15, 2024 •

edited

Loading

jackwener Jan 16, 2024

jackwener left a comment

alamb left a comment

alamb Jan 16, 2024

viirya Jan 17, 2024

alamb Jan 16, 2024

jackwener Jan 16, 2024 •

edited

Loading

alamb Jan 16, 2024

alamb left a comment

alamb Jan 17, 2024

viirya Jan 17, 2024

viirya commented Jan 19, 2024 •

edited

Loading

alamb commented Jan 22, 2024

alamb commented Jan 22, 2024

viirya commented Jan 22, 2024

dadepo commented Jan 23, 2024

viirya commented Jan 23, 2024 •

edited

Loading

	Ok(ColumnarValue::Array(Arc::new(array.clone()) as ArrayRef))
	Ok(ColumnarValue::from(Arc::new(array.clone()) as ArrayRef))

	let ColumnarValue::Array(arg0) = &args[0] else {
	let arg0 = args[0].into_array(num_rows)

Deprecate make_scalar_function #8878

Deprecate make_scalar_function #8878

Conversation

viirya commented Jan 15, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

viirya Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

jackwener Jan 16, 2024

Choose a reason for hiding this comment

jackwener left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 16, 2024

Choose a reason for hiding this comment

viirya Jan 17, 2024

Choose a reason for hiding this comment

alamb Jan 16, 2024

Choose a reason for hiding this comment

jackwener Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Jan 16, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 17, 2024

Choose a reason for hiding this comment

viirya Jan 17, 2024

Choose a reason for hiding this comment

viirya commented Jan 19, 2024 • edited Loading

alamb commented Jan 22, 2024

alamb commented Jan 22, 2024

viirya commented Jan 22, 2024

dadepo commented Jan 23, 2024

viirya commented Jan 23, 2024 • edited Loading

viirya Jan 15, 2024 •

edited

Loading

jackwener Jan 16, 2024 •

edited

Loading

viirya commented Jan 19, 2024 •

edited

Loading

viirya commented Jan 23, 2024 •

edited

Loading